Skip to content

Update ARCHITECTURE.md with latest revisions and high-level proposed HTTP approach #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

felixarntz
Copy link
Member

This PR revises a few things in ARCHITECTURE.md, partially in accordance with revisions already implemented, partially as a proposed approach based on recent conversations about the HTTP infrastructure layer.

Here's a breakdown:

  • Update the interface for operations based on recent code revisions. See b409875
  • Update the file API layer based on how it was eventually implemented. See 85eea9c
  • Fix minor incorrection with function call and function response DTOs. See 8a200f7
  • Revise proposed HTTP infrastructure, and related authentication layer. See dd245b1
    • I created this after reviewing the in progress work in trunk...add-http-client-and-auth from @JasonTheAdams, with minor modifications. Let's discuss these here.
    • This is supposed to be only a high-level summary in line with the rest of the existing class diagrams, i.e. not as detailed as what that branch is adding.

Note: There are 3 very minor fixes in actual code files that I noticed were needed while reviewing the architecture diagram.

@felixarntz felixarntz added the [Type] Developer Documentation Documentation for developers label Aug 11, 2025
@felixarntz
Copy link
Member Author

@JasonTheAdams Let's discuss the minor differences between your proposed HTTP architecture outline in trunk...add-http-client-and-auth and what I have here:

  1. I think all of it should live within the Providers namespace, in an Http sub-namespace of it. That's because they're purely relevant for the extender API, and putting all such code into Providers was something we initially went for.
  2. I included a getData method in Request and Response, which makes it easy to work with certain common payloads. How that data is handled under the hood is an implementation detail.
    • For example, in a GET request, get_data() would return the key value pairs for what's going to be eventually sent as query parameters.
    • For example, in a JSON response, get_data() would return the associative array of JSON-decoded data.
    • When irrelevant, e.g. in a response for a binary file, get_data() would simply return null.
  3. getBody may return null, as it may not always be present. The idea is that depending on the type of request or response, you either expect a body (raw string) or data (associative data array).
  4. getHeaders always returns array<string, string[]>, instead of having the values be string|string[], which is error-prone as it would require any caller to check whether the data is an array or a single string. Choosing one and always going with it is more straightforward to use.
  5. I left out the more specific getReasonPhrase and isSuccessful methods in Response. IMO they're too opinionated to include in the main DTO, at least for now.
  6. I left out the with* methods in Request. I'm not sure we need them, and if we actually do, this would be a new pattern we don't have anywhere else. Maybe this is how we would want to do it, maybe have a RequestBuilder instead. Anyway, I think this would be a premature discussion at this point, since again I'm not sure we even need it.

Let me know your thoughts on these changes.

FWIW I also revised Authentication to be more clearly tied to the Http infrastructure, also by renaming it to RequestAuthentication. This is not a change from what your PR is doing, since it doesn't touch those classes, but still wanted to highlight it here.

@JasonTheAdams
Copy link
Member

Hi @felixarntz! 👋

I had a feeling you'd want to move Http as a sub-domain of the Providers namespace. Hahah! I figured I'd just start and then move it if/when you suggested it. 😄

One thing I was debating is whether it simply makes sense to update our Request and Response classes to implement the PSR-7 RequestInterface and ResponseInterface interfaces, respectively. It's not a big deal to add the few methods they require and then it removes the need for any kind of conversion step. They can just be passed to and from the client. Well, I guess there'd need to be a conversion step coming from the client for requests, but that's not a big deal. What do you think?

Otherwise, I'm good with everything you're describing add/modifying! 👍

@felixarntz
Copy link
Member Author

@JasonTheAdams

One thing I was debating is whether it simply makes sense to update our Request and Response classes to implement the PSR-7 RequestInterface and ResponseInterface interfaces, respectively. It's not a big deal to add the few methods they require and then it removes the need for any kind of conversion step. They can just be passed to and from the client. Well, I guess there'd need to be a conversion step coming from the client for requests, but that's not a big deal. What do you think?

I'd prefer keeping them decoupled. Since the conversion between our DTOs and their PSR-7 equivalents is only relevant at the transporter layer, I think it's enough of an implementation detail that it shouldn't cause relevant overhead. But decoupling ensures that we have full control over the shape of our DTOs, while being PSR-7 interoperable via conversion. I think it's also a bit more flexible and future-proof as we could dynamically convert our DTOs to different PSR-7 interface implementations depending on what's available (e.g. Guzzle or something else).

@JasonTheAdams
Copy link
Member

I'd prefer keeping them decoupled. Since the conversion between our DTOs and their PSR-7 equivalents is only relevant at the transporter layer, I think it's enough of an implementation detail that it shouldn't cause relevant overhead. But decoupling ensures that we have full control over the shape of our DTOs, while being PSR-7 interoperable via conversion.

That's fair. It certainly does keep our DTOs simple and specific to our needs.

I think it's also a bit more flexible and future-proof as we could dynamically convert our DTOs to different PSR-7 interface implementations depending on what's available (e.g. Guzzle or something else).

I don't think this is exactly true. Guzzle and such don't really care so long as they use PSR-7 interfaces. We don't need to worry about converting to different PSR-7 implementations, as that's the point of PSR-7. Other libraries would do what we're doing and convert to/from their own objects.

Not using PSR-7 on our objects really only has the benefit that we don't have extra methods. I don't think it makes any difference with how we interface with the client libraries. So either we implement PSR-7 Request and Response interfaces on our objects, or else we have conversion steps. Not a big deal either way, in my opinion.

@felixarntz
Copy link
Member Author

I think it's also a bit more flexible and future-proof as we could dynamically convert our DTOs to different PSR-7 interface implementations depending on what's available (e.g. Guzzle or something else).

I don't think this is exactly true. Guzzle and such don't really care so long as they use PSR-7 interfaces. We don't need to worry about converting to different PSR-7 implementations, as that's the point of PSR-7. Other libraries would do what we're doing and convert to/from their own objects.

I partially agree. Guzzle itself doesn't care what you pass, but still Guzzle also has its own implementation, see e.g. https://github.com/guzzle/psr7/blob/2.7/src/Request.php. So we could also use whichever PSR-7 request implementation is present instead of more or less writing the same code ourselves.

Not using PSR-7 on our objects really only has the benefit that we don't have extra methods. I don't think it makes any difference with how we interface with the client libraries. So either we implement PSR-7 Request and Response interfaces on our objects, or else we have conversion steps. Not a big deal either way, in my opinion.

I'd prefer the conversion steps approach, just because it keeps our code more independent. For instance, I don't love that PSR-7 requires a UriInterface to be passed. For our implementation, I'd prefer for that to be a simple URI string, and we can parse it internally as needed, e.g. as part of the conversion.

@JasonTheAdams
Copy link
Member

I'm cool with the conversion strategy! 😄

Regarding Guzzle, I feel like we agree and are somehow talking around each other. Hahah! In any case, yes, PSR-7 with the discovery system for Guzzle (and other) integration via PSR-17 and PSR-18. I think what I built in #47 moves us in that direction. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants